Skip to content

Add Node.js v8.0.0#411

Merged
Starefossen merged 1 commit into
nodejs:masterfrom
SimenB:node-8-del-7
May 31, 2017
Merged

Add Node.js v8.0.0#411
Starefossen merged 1 commit into
nodejs:masterfrom
SimenB:node-8-del-7

Conversation

@SimenB

@SimenB SimenB commented May 30, 2017

Copy link
Copy Markdown
Member

#410 with node 7 removed. Unsure which of these (if any) you want 😀

@SimenB SimenB mentioned this pull request May 30, 2017
@chorrell

Copy link
Copy Markdown
Contributor

The Docker files should be in a 8.0/ directory.

@SimenB

SimenB commented May 30, 2017

Copy link
Copy Markdown
Member Author

The Docker files should be in a 8.0/ directory.

They are.
image

@chorrell

Copy link
Copy Markdown
Contributor

Ha! I missed that.

In that case LGTM

@SimenB

SimenB commented May 30, 2017

Copy link
Copy Markdown
Member Author

You should enable cancelling old builds on new pushes on travis. I force pushed a bit, and the queue is real right now 😭

https://blog.travis-ci.com/2017-03-22-introducing-auto-cancellation

@chorrell

Copy link
Copy Markdown
Contributor

I would if I had the access to do that ;-)

Someone from the build group should be able to enable that for us.

@SimenB

SimenB commented May 30, 2017

Copy link
Copy Markdown
Member Author

Should I close #410?

@chorrell

Copy link
Copy Markdown
Contributor

Sure.

Starefossen

This comment was marked as off-topic.

@pesho

pesho commented May 30, 2017

Copy link
Copy Markdown
Contributor

Wait, is 7.x a completely abandoned branch now?

@SimenB

SimenB commented May 30, 2017

Copy link
Copy Markdown
Member Author

Wait, is 7.x a completely abandoned branch now?

https://github.com/nodejs/LTS/blob/4af640885c5888894348ace2b15942e8989494e4/README.md

An odd-numbered major release will cease to be actively updated when the subsequent even-numbered major release is cut.

Not really sure if that means "completely abandoned", though...

Btw, can one of you cancel some of the builds here? https://travis-ci.org/nodejs/docker-node/pull_requests (bonus is activating auto cancellation 🙂)

@pesho

pesho commented May 30, 2017

Copy link
Copy Markdown
Contributor

@SimenB thanks, it is indeed abandoned then.

I think the Yarn version should be updated too in this PR, according to the (not yet officially accepted) policy in #393

@chorrell

Copy link
Copy Markdown
Contributor

Were we also going to update Alpine for 8.x or was there a different plan for that? Either way, that can probably be handled separately.

@pesho

pesho commented May 30, 2017

Copy link
Copy Markdown
Contributor

Yes, Alpine should also be updated (probably to 3.6) according to the discussion in #399

@SimenB

SimenB commented May 30, 2017

Copy link
Copy Markdown
Member Author

I think the Yarn version should be updated too in this PR

I chose not to update it to keep it consistent between images. That doesn't really make sense though, the npm version is already different. Want me to update it?

@Starefossen

Starefossen commented May 30, 2017

Copy link
Copy Markdown
Member

I'd vote we merge this PR as is and make a separate ones for upgrade of Alpine and Yarn.

@pesho

pesho commented May 30, 2017

Copy link
Copy Markdown
Contributor

I chose not to update it to keep it consistent between images.

Running ./update.sh 8.0 should "Do the right thing":tm: and only update it for 8.x.

@pesho

pesho commented May 30, 2017

Copy link
Copy Markdown
Contributor

I'm ok with doing Yarn and Alpine separately in this case, but in general think they should be handled in a single PR, atomically.

@SimenB

SimenB commented May 30, 2017

Copy link
Copy Markdown
Member Author

CI is green, but it doesn't seem connected to the latest commit in this PR... https://travis-ci.org/nodejs/docker-node/builds/237656473

@pouwerkerk

Copy link
Copy Markdown

@SimenB looks like the commit that CI built is the merge commit: 8e936c8

@olalonde

Copy link
Copy Markdown

💤

@xzyfer xzyfer mentioned this pull request May 31, 2017
6 tasks
@SimenB

SimenB commented May 31, 2017

Copy link
Copy Markdown
Member Author

CI is confused, I force pushed the same commit now.

EDIT: Didn't help at all... I have no idea, just merge it? The same diff was green even if github/travis is a bit confused

@Starefossen

Copy link
Copy Markdown
Member

I checked locally, it's all good. Merging.

@Starefossen Starefossen merged commit 82c21d3 into nodejs:master May 31, 2017
@SimenB SimenB deleted the node-8-del-7 branch May 31, 2017 06:04
@SimenB

SimenB commented May 31, 2017

Copy link
Copy Markdown
Member Author

Great! Will this be pushed to the docker registry, or should yarn and alpine be updated first?

EDIT: PR for the former: #412, latter: #413

@Starefossen

Starefossen commented May 31, 2017

Copy link
Copy Markdown
Member

Thanks a lot @SimenB. I'll merge them as soon as the builds finish. I have prepared a PR to Docker Hub. Hopefully will get this one out buy by the end of the day.

tianon pushed a commit to docker-library/official-images that referenced this pull request May 31, 2017
PR-URL #2997

Related: nodejs/node#12220 nodejs/docker-node#411 nodejs/docker-node#412 nodejs/docker-node#113

Signed-off-by: Hans Kristian Flaatten <hans@starefossen.com>
@dy-dx dy-dx mentioned this pull request Jul 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants